Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

only use env_logger in binaries #3276

Merged

Conversation

tdelabro
Copy link
Contributor

@tdelabro tdelabro commented May 30, 2023

env_logger is used in the cairo-lang crates, a bit indiscriminately in libs and bin.

The crate documentation states that it should only be used in binaries. This PR makes sure it does.

The problem is that those binaries are part of the same crates as the lib they use, making them share the same [dependencies]. This is problematic because we don't want it imported at all while building the lib, but we want it when building the binary.

There is multiple ways to fix this, as explained in this StackOverflow answer. I chose the second option: defining a specific feature for this import and adding it as required-features in the [[bin]] section.

I did it because it is the less invasive solution for now. But my recommendation would be to separate entirely binaries from libs, by making them exist in a separate crate where we can have full control over their dependencies (it will be more handy when we start to support no_std builds).

Wdyt? Is it good enough for now? Do you prefer the standalone binary crate solutions? If so where should those crates exist, and how should they be named (<name of the lib>-bin? Something else?).


This change is Reviewable

@tdelabro tdelabro force-pushed the only-use-env_logger-in-binaries branch from 3fab467 to ebb6754 Compare May 30, 2023 19:03
@tdelabro
Copy link
Contributor Author

@spapinistarkware Following the exchange you had with @abdelhamidbakhta about no_std support in the rust cairo libraries, I started working on it.
This is the first thing that will require a change (env_logger not being no_std compatible).

@tdelabro tdelabro force-pushed the only-use-env_logger-in-binaries branch from ebb6754 to 96ee199 Compare May 30, 2023 19:07
Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @tdelabro)

@spapinistarkware
Copy link
Contributor

spapinistarkware commented May 30, 2023 via email

auto-merge was automatically disabled May 30, 2023 22:55

Head branch was pushed to by a user without write access

@tdelabro tdelabro force-pushed the only-use-env_logger-in-binaries branch from 29194bb to 059cdc1 Compare May 30, 2023 22:56
@tdelabro tdelabro force-pushed the only-use-env_logger-in-binaries branch from 059cdc1 to bb6a341 Compare May 30, 2023 22:59
Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tdelabro)


scripts/cairo_fmt.sh line 3 at r2 (raw file):

#!/bin/bash

cargo run --bin cairo-format --features="binary" -- --recursive "$@"

Do I need to specify this when I run? Why is that, I thought defining in the toml is enough.
I'd really like to avoid this.

@tdelabro
Copy link
Contributor Author

tdelabro commented May 31, 2023

Yes, you will have to. The feature is required, meaning compilation won't even start if it's not there, but that does not mean that it is automatically used.

Two solutions:

  • add a default = ["binary"] feature (but it will also be the default for the lib meaning that using default-features = false while importing the lib in another crate will be kinda mandatory)
  • put the binaries in their own crates and all those shenanigans will not longer be required, because we can just import what need

Again I think in the long run, having the binaries in separate folders will be clearer and easier to maintain

The discussion about the functionality we would like to have are being hold here, with an RFC being redacted here

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I believe extracting binaries to a separate crate is a way to go upfront here. I suspect this play with features will bring issues to compiler crates users :/

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tdelabro)

Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tdelabro Can you try to separate the binaries into different crate then?
Remember to add the new crate to the ci yml file.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tdelabro)

@tdelabro tdelabro force-pushed the only-use-env_logger-in-binaries branch from d4d274a to 4ab2770 Compare May 31, 2023 14:33
@tdelabro
Copy link
Contributor Author

@spapinistarkware done.
I put all binaries under a crates/bin/ folder

Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 29 of 29 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tdelabro)

@tdelabro
Copy link
Contributor Author

@spapinistarkware should I rebase and you merge?
or do we want multiple reviewers?
Is there some specific policies on those repositories?

@spapinistarkware
Copy link
Contributor

No need to rebase. I want @orizi to also tal, and then we'll merge.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 9 files at r1, 2 of 8 files at r2, 27 of 29 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tdelabro)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tdelabro)

@tdelabro
Copy link
Contributor Author

tdelabro commented Jun 1, 2023

@spapinistarkware ok perfect

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tdelabro)


crates/cairo-lang-language-server/src/lib.rs line 72 at r3 (raw file):

pub async fn serve_language_service() {
    init_logging(log::LevelFilter::Warn);

@maciektr please take a look at this, I think we will have to adapt Scarb's LS to also include logging, perhaps via tracing which we use there?

@spapinistarkware spapinistarkware added this pull request to the merge queue Jun 1, 2023
Merged via the queue into starkware-libs:main with commit 7cbc6cf Jun 1, 2023
@tdelabro tdelabro deleted the only-use-env_logger-in-binaries branch June 1, 2023 14:54
mkaput pushed a commit that referenced this pull request Jun 2, 2023
mkaput pushed a commit that referenced this pull request Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants